Fix memory pool lifetime for Arrow stream buffers - #464
Conversation
There was a problem hiding this comment.
Pull request overview
Note
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Fixes a use-after-free risk where Arrow stream buffers can outlive the arrow::MemoryPool used to allocate them, especially when readers are closed while callers still hold buffers (or async reads complete after close).
Changes:
- Keep the
arrow::MemoryPoolalive for the lifetime of returnedarrow::Bufferobjects usingshared_ptraliasing. - Add deterministic unit tests covering
Read,ReadAt, andReadAsyncwith a minimal deferredInputStreammock.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/paimon/common/utils/arrow/arrow_stream_adapter_test.cpp | Adds a deferred/mock input stream and new tests that ensure buffers remain valid after the adapter/pool are destroyed. |
| src/paimon/common/utils/arrow/arrow_input_stream_adapter.cpp | Wraps returned buffers so they retain ownership of the memory pool until buffer destruction. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
a0dca34 to
89c5009
Compare
dd772d5 to
6e4fa8e
Compare
Keep the Arrow memory pool alive while stream buffers remain referenced. Add deterministic coverage for synchronous and asynchronous reads after adapter teardown. Co-authored-by: GPT-5.6 Terra <codex@users.noreply.github.com>
6e4fa8e to
5572ac1
Compare
| return arrow::Status::OK(); | ||
| } | ||
|
|
||
| struct BufferWithMemoryPool { |
There was a problem hiding this comment.
We have a performance concern, as this would increment the shared_ptr reference count on every I/O operation. Have you evaluated the performance overhead of introducing shared_ptr here?
There was a problem hiding this comment.
I ran a pure in-memory microbenchmark. Results (before -> after, ns/op):
| Read size | Read | ReadAt | ReadAsync |
|---|---|---|---|
| 4 KiB, 1 thread | +10.4% | +10.9% | +7.9% |
| 64 KiB, 1 thread | +3.7% | +8.4% | +4.6% |
| 1 MiB, 1 thread | +0.0% | -1.2% | +0.1% |
The overhead becomes smaller as the read size increases. The fix is still needed to keep the memory pool alive while returned buffers are referenced; otherwise, the ownership between the pool, reader, and buffers would need to change.
|
Could you please move this PR to https://github.com/apache/paimon-cpp? All future development will take place under the Apache repo. |
Yes, but should we move S3 related stuff first? |
I found this while debugging an S3 crash. I initially suspected the S3 filesystem, but the issue was in the Arrow input-stream adapter: a returned buffer can outlive its memory pool when a reader is closed while a caller still holds a buffer. This is more likely with network filesystems, where asynchronous reads may finish after the reader is closed.
This patch uses
shared_ptr's aliasing constructor to keep the pool alive with the returned buffer. It also adds deterministic coverage forRead,ReadAt, andReadAsyncusing a smallInputStreammock. Without the change, the tests crash; with it, they pass.